Skip to content

Conversation

prdoyle
Copy link
Contributor

@prdoyle prdoyle commented Mar 18, 2025

See ES-11273

Problem

StackWalker.getCallerClass is specified to throw IllegalCallerException when it is "called from a method which is the last frame on the stack". It turns out this includes situations where a method is called via JNI from a native method. @smalyshev observed that, when attaching a debugger using server=y, a native method calls Thread.isDaemon, and our instrumentation inside the latter calls getCallerClass which throws.

Rejected solutions

The most obvious remedy would be to write a wrapper method that calls getCallerClass and catches the exception; however, this would add an extra stack frame that would cause getCallerClass to return the wrong frame.

Inserting a catch into the instrumentation bytecode is a possibility, but it would dramatically increase the complexity of the instrumentation, which is currently quite straightforward.

Selected solution

I've opted to implement our own getCallerClass with the behviour we want, as a drop-in replacement for the one from StackWalker. Instead of throwing when there's no caller frame, it returns a marker class NO_CLASS to indicate that condition.

@prdoyle prdoyle added auto-backport Automatically create backport pull requests when merged test-entitlements v8.18.1 v8.19.0 v9.0.1 v9.1.0 :Core/Infra/Entitlements Entitlements infrastructure labels Mar 18, 2025
@prdoyle prdoyle self-assigned this Mar 18, 2025
@smalyshev
Copy link
Contributor

For me it fixes the debugger connection problem. Don't understand it enough to approve but at least it seems to works in my case.

@prdoyle prdoyle marked this pull request as ready for review March 19, 2025 17:42
@prdoyle prdoyle requested a review from a team as a code owner March 19, 2025 17:42
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Mar 19, 2025
Copy link
Contributor

@ldematte ldematte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you expand the PR description? Without that it's very difficult what this PR does and why.

@prdoyle
Copy link
Contributor Author

prdoyle commented Mar 20, 2025

Sorry @ldematte! Forgot to update the description when I moved this out of Draft.

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I suppose a test for the empty walk case isn't really possible?

@prdoyle prdoyle merged commit b8c70ae into elastic:main Apr 1, 2025
22 checks passed
@prdoyle prdoyle deleted the caller-class branch April 1, 2025 18:45
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
8.18
8.x
9.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-backport Automatically create backport pull requests when merged :Core/Infra/Entitlements Entitlements infrastructure >non-issue Team:Core/Infra Meta label for core/infra team v8.18.1 v8.19.0 v9.0.1 v9.1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants